Codecov Report

Attention: Patch coverage is 97.10983% with 5 lines in your changes are missing coverage. Please review.

Project coverage is 84.46%. Comparing base (450dfe3) to head (2661dda).
Report is 43 commits behind head on main.

@@            Coverage Diff             @@
##             main    #3347      +/-   ##
==========================================
- Coverage   85.26%   84.46%   -0.80%     
==========================================
  Files         881      893      +12     
  Lines      143843   147284    +3441     
==========================================
+ Hits       122641   124406    +1765     
- Misses      21202    22878    +1676     

Looks cool.

I have a question: assuming a user wants to partition data by date, for example, one partition per day, is this syntax currently supported?

I have a question: assuming a user wants to partition data by date, for example, one partition per day, is this syntax currently supported?

This is not supported in this place (or one can manually enumerate the partition, but this isn't what we want).

Maybe we can implement some special rule for TIME INDEX in the future? Like

PARTITION ON COLUMNS (..., ts) (
  ...,
  ts WITHIN '1d'
)

But is it a good practice to include TIME INDEX in the partition columns?

Looks cool.

I have a question: assuming a user wants to partition data by date, for example, one partition per day, is this syntax currently supported?

It seems that this mode is not supported now, and it will make the number of regions dynamic. This also has a certain impact on the structure of routing info. There may also be some inactive regions, which means that no new data is being written.
I have a little concern about the above question, and I am slightly doubtful whether this mode is really necessary, given that our DB naturally organizes data according to timestamps.

Maybe we can implement some special rule for TIME INDEX in the future? Like

PARTITION ON COLUMNS (..., ts) (
  ...,
  ts WITHIN '1d'
)

That would be great!

But is it a good practice to include TIME INDEX in the partition columns?

Yes, it's not a good practice to partition data by TIME INDEX in common. But in some use cases, there is not too much data, and users want to organize and manage the data by date.

BTW, Do we need to add some alias for each <, >, >= , like adding lt, less than for <. (Maybe in another PR)

1. Partitioning by date like that requires creating region on demand. It's not trivial work. However, once we have dynamic partitioning, it should be easy to Implement this static partitioning. I suggest we take a consideration of starting from there.
2. Looking at the old partition grammar, I find the new grammar a little less interesting, more of like a "syntactic sugar". For example, the old:

CREATE TABLE dist_table(
    ts TIMESTAMP DEFAULT current_timestamp(),
    n INT,
    row_id INT,
    PRIMARY KEY(n),
    TIME INDEX (ts)
)
PARTITION BY RANGE COLUMNS (n) (
    PARTITION r0 VALUES LESS THAN (5),
    PARTITION r1 VALUES LESS THAN (9),
    PARTITION r2 VALUES LESS THAN (MAXVALUE),
)
engine=mito;

versus the new:

CREATE TABLE dist_table(
    ts TIMESTAMP DEFAULT current_timestamp(),
    n INT,
    row_id INT,
    PRIMARY KEY(n),
    TIME INDEX (ts)
)
PARTITION ON (n) (
    n < 5,
    n < 10,
    n >= 10,
)
engine=mito;

It's of course good to have this simplification, and I'm ok with this new grammar. However, I'm afraid if the partition rules grow, or the partition methods like list or hash being supported, in the end the new grammar would have became just as obscure as the old.

3. Partitioning by time is natural to human, and is common in tsdb. At least timescaledb and influxdb both first decide to do it. Originally there's a hotspot issue we are concerning of. But now under the reproposing of partition grammar, I think it's time to reconsider

Yes, it's not a good practice to partition data by TIME INDEX in common. But in some use cases, there is not too much data, and users want to organize and manage the data by date.

Partitioning by time is natural to human, and is common in tsdb. At least timescaledb and influxdb both first decide to do it. Originally there's a hotspot issue we are concerning of. But now under the reproposing of partition grammar, I think it's time to reconsider

To remind, our database has the same capability corresponding to hypertables from TimescaleDB - or as a non-pg-based storage system, we simply do not have that issue that needs "hypertable".

Data would be organized to appropriate time windows during compaction, automatically. What can benefit more from encouraging users to set a time span?

BTW, Do we need to add some alias for each <, >, >= , like adding lt, less than for <. (Maybe in another PR)

Here uses the same parser with WHERE clause. Alias like <> for != is available. lt and less than can be chosen if we want more than WHERE.

However, I'm afraid if the partition rules grow, or the partition methods like list or hash being supported, in the end the new grammar would have became just as obscure as the old.

That's a good point. There should be a grammar to define repeated rules (like the time partition). I added a task to the tracker

The old grammar comes from mysql, it's complex but complete. If the new grammar looks just like the old, I'm afraid we are wasting time reinventing it. So I'm neutral to this change. I say let's design a whole new syntax for the unique dynamic partitioning feat for GreptimeDB!

The integration test always fails @waynexia
https://github.com/GreptimeTeam/greptimedb/actions/runs/8050032205/job/21984750684